Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use a Dict instead of an IdDict for caching of the cwstring for Windows env variables #52758

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

KristofferC
Copy link
Sponsor Member

Should fix #52711.

My analysis of the invalidation is as follows:

We added code to cache the conversion to cwstring in env handling on Windows (#51371):

const env_dict = IdDict{String, Vector{UInt16}}()

function memoized_env_lookup(str::AbstractString)
    ...
    env_dict[str] = cwstring(str)
    ...
end

function access_env(onError::Function, str::AbstractString)
    var = memoized_env_lookup(str)
    ...
end

Since IdDict has @nospecialize on setindex! we compile this method:

setindex!(::IdDict{String, Vector{UInt16}}, ::Any, ::Any)

which has an edge to:

convert(Type{Vector{Int64}}, Any})

But then StaticArrays comes along and adds a method

convert(::Type{Array{T, N}}, ::StaticArray)

which invalidates the setindex! (due to the edge to convert) which invalidates the whole env handling on Windows which causes 4k other methods downstream to be invalidated, in particular the artifact string macro which causes a significant delay in the next jll package you load after loading StaticArrays.

There should be no performance penalty to this since strings already does a hash for their objectid.

cc @jaakkor2, could you try this PR out?

cc @MasonProtter

…dows env variables

`IdDict` methods have a bunch of `nospecialize` annotations which specifically when using an `Array` value causes methods to be compiled that are very vulnerable to invalidations.
@KristofferC KristofferC added the backport 1.10 Change should be backported to the 1.10 release label Jan 5, 2024
@vtjnash vtjnash added status:merge me PR is reviewed. Merge when all tests are passing and removed status:merge me PR is reviewed. Merge when all tests are passing labels Jan 5, 2024
vtjnash
vtjnash previously requested changes Jan 5, 2024
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike IdDict, the Dict code is not thread-safe for concurrent lookups, so will need to move those lookup access behind a lock

@MasonProtter
Copy link
Contributor

MasonProtter commented Jan 5, 2024

moving the accesses behind a lock could slow things down a bit right? Choosing dict types based just on opaque non-local invalidation effects seems like a kinda unhappy situation. I wonder if there's a way to keep the IdDict but avoid the invalidation

@KristofferC
Copy link
Sponsor Member Author

the Dict code is not thread-safe for concurrent lookups

For my learning, what causes this unsafeness?

Choosing dict types based just on opaque non-local invalidation effects seems like a kinda unhappy situation.

I agree, but the current situation is also unhappy.

I wonder if there's a way to keep the IdDict but avoid the invalidation

The memoized_env_lookup could be moved behind an inference barrier but that also has some performance implications. However, the speed of the caching makes lookups faster than on 1.9 anyway so perhaps that is still ok?

@KristofferC
Copy link
Sponsor Member Author

#52760 for example.

@IanButterworth
Copy link
Sponsor Member

What about going back to the non-Dict original form of #51371 and just store the cwstring for JULIA_DEBUG? That seems like a safe fix for a 1.10.1 ?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 5, 2024

The lock performance should be fine. The function you call here (the environment variables lookup) also already always needed to take a lock internally on all platforms, so the added overhead should be slight.

@jaakkor2
Copy link
Contributor

jaakkor2 commented Jan 5, 2024

Lock-version does not compile, output var is not in the scope.
https://github.com/JuliaLang/julia/blob/kc/dict_fdsfds/base/env.jl#L20

I defined var = nothing before the @lock, and then it does fix the invalidation issue.

@KristofferC KristofferC merged commit b7c24ed into master Jan 8, 2024
5 of 7 checks passed
@KristofferC KristofferC deleted the kc/dict_fdsfds branch January 8, 2024 13:17
KristofferC added a commit that referenced this pull request Jan 24, 2024
…dows env variables (#52758)

Should fix #52711.

My analysis of the invalidation is as follows:

We added code to cache the conversion to `cwstring` in env handling on
Windows (#51371):

```julia
const env_dict = IdDict{String, Vector{UInt16}}()

function memoized_env_lookup(str::AbstractString)
    ...
    env_dict[str] = cwstring(str)
    ...
end

function access_env(onError::Function, str::AbstractString)
    var = memoized_env_lookup(str)
    ...
end
```

Since `IdDict` has `@nospecialize` on `setindex!` we compile this
method:

```julia
setindex!(::IdDict{String, Vector{UInt16}}, ::Any, ::Any)
```

which has an edge to:

```julia
convert(Type{Vector{Int64}}, Any})
```

But then StaticArrays comes along and adds a method

```julia
convert(::Type{Array{T, N}}, ::StaticArray)
```

which invalidates the `setindex!` (due to the edge to `convert`) which
invalidates the whole env handling on Windows which causes 4k other
methods downstream to be invalidated, in particular, the artifact string
macro which causes a significant delay in the next jll package you load
after loading StaticArrays.

There should be no performance penalty to this since strings already
does a hash for their `objectid`.

(cherry picked from commit b7c24ed)
@KristofferC KristofferC mentioned this pull request Jan 24, 2024
33 tasks
KristofferC added a commit that referenced this pull request Feb 6, 2024
Backported PRs:
- [x] #51095 <!-- Fix edge cases where inexact conversions to UInt don't
throw -->
- [x] #52583 <!-- Don't access parent of triangular matrix in powm -->
- [x] #52645 <!-- update --gcthreads section in command line options -->
- [x] #52423 <!-- update nthreads info in versioninfo -->
- [x] #52721 <!-- inference: Guard TypeVar special case against vararg
-->
- [x] #52637 <!-- fix finding bundled stdlibs even if they are e.g.
devved in an environment higher in the load path -->
- [x] #52752 <!-- staticdata: handle cycles in datatypes -->
- [x] #52758 <!-- use a Dict instead of an IdDict for caching of the
`cwstring` for Windows env variables -->
- [x] #51375 <!-- Insert hardcoded backlinks to stdlib doc pages -->
- [x] #52994 <!-- place work-stealing queue indices on different cache
lines to avoid false-sharing -->
- [x] #53015 <!-- Add type assertion in iterate for logicalindex -->
- [x] #53032 <!-- Fix a list in GC devdocs -->
- [x] #52748 
- [x] #52856 
- [x] #52878
- [x] #52754 
- [x] #52228
- [x] #52924
- [x] #52569 <!-- Fix GC rooting during rehashing of iddict -->
- [x] #52605 <!-- Default uplo in symmetric/hermitian -->
- [x] #52618 <!-- heap snapshot: add gc roots and gc finalist roots to
fix unrooted nodes -->
- [x] #52781 <!-- fix type-stability bugs in Ryu code -->
- [x] #53055 <!-- Profile: use full terminal cols to show function name
-->
- [x] #53096 
- [x] #53076 
- [x] #52841 <!-- Extensions: make loading of extensions independent of
what packages are in the sysimage -->
- [x] #52078 <!-- Replace `&hArr;` by `&harr;` in documentation -->
- [x] #53035 <!-- use proper cache-line size variable in work-stealing
queue -->
- [x] #53066 <!-- doc: replace harr HTML entity by unicode -->
- [x] #52996 <!-- Apple silicon has 128 byte alignment so fix our
defines to match -->
- [x] #53121 

Non-merged PRs with backport label:
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Feb 6, 2024
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
…dows env variables (JuliaLang#52758)

Should fix JuliaLang#52711.

My analysis of the invalidation is as follows:

We added code to cache the conversion to `cwstring` in env handling on
Windows (JuliaLang#51371):

```julia
const env_dict = IdDict{String, Vector{UInt16}}()

function memoized_env_lookup(str::AbstractString)
    ...
    env_dict[str] = cwstring(str)
    ...
end

function access_env(onError::Function, str::AbstractString)
    var = memoized_env_lookup(str)
    ...
end
```

Since `IdDict` has `@nospecialize` on `setindex!` we compile this
method:

```julia
setindex!(::IdDict{String, Vector{UInt16}}, ::Any, ::Any)
```

which has an edge to:

```julia
convert(Type{Vector{Int64}}, Any})
```

But then StaticArrays comes along and adds a method

```julia
convert(::Type{Array{T, N}}, ::StaticArray)
```

which invalidates the `setindex!` (due to the edge to `convert`) which
invalidates the whole env handling on Windows which causes 4k other
methods downstream to be invalidated, in particular, the artifact string
macro which causes a significant delay in the next jll package you load
after loading StaticArrays.

There should be no performance penalty to this since strings already
does a hash for their `objectid`.

(cherry picked from commit b7c24ed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in package loading on Windows starting in Julia 1.10.0-rc2
5 participants